-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Assert more about the test server fixture #140
Conversation
"//depot/file.txt": "Hello World\n", | ||
"//stream-depot/main/file.txt": "Hello Stream World\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content of files
assert submitted_changeinfo == { | ||
'1' :{ | ||
'action': ['add'], | ||
'depotFile': ['//depot/file.txt'], | ||
'desc': 'Initial Commit' | ||
}, | ||
'2' :{ | ||
'action': ['add'], | ||
'depotFile': ['//stream-depot/main/file.txt'], | ||
'desc': 'Initial Commit to Stream\n' | ||
}, | ||
'6' :{ | ||
'action': ['edit'], | ||
'depotFile': ['//depot/file.txt'], | ||
'desc': 'modify //depot/file.txt\n' | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If tests fail, it shows you a rich diff of exactly which bits were different
'action': ['edit'], | ||
'depotFile': ['//depot/file.txt'], | ||
'desc': 'Modify file in shelved change\n', | ||
# Change content from 'Hello World\n' to 'Goodbye World\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully the comment suffices here
If there are more complex shelved changes created we could look at fetching the content of the shelved change and including it here. Omitted for the sake of simplicity for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually on second though this might be really useful to include the content, as it means we can put this static info in a global var for the unit tests and refer to it from throughout, avoiding potential mistakes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in a future PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be up for making that, it's been a while since I python'd.
@commanderofthegrey Could you take a look at this? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; are other EV reviewing too, to learn?
} | ||
|
||
# Check submitted changes | ||
submitted_changes = [change for change in repo.perforce.run_changes('-s', 'submitted')] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a long-form flag instead of -s
, for readability?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately not, it means status
|
||
# Check shelved changes | ||
shelved_changes = [change for change in repo.perforce.run_changes('-s', 'pending')] | ||
shelved_changeinfo = {change["change"]: repo.perforce.run_describe('-S', change["change"])[0] for change in shelved_changes} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long form for -S
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately not, it means get info for shelved files on this change
hopefully this is implied from the var it is being assigned to
I'm not sure I understand enough what is going on... :/ |
We have a zip of a perforce server which is used to run unit tests against and this asserts around some state it has, otherwise its a bit of a 'black box' as to what is on the server |
Improve code-as-documentation on what the server fixture contains
In lieu of work on issue #111